HDDS-15849. Force channel shutdown in XceiverClientGrpc.close#10747
Conversation
|
cc @ptlrs |
There was a problem hiding this comment.
Pull request overview
This PR addresses the HDDS-15849 performance regression by removing the fixed 100ms polling sleep from XceiverClientGrpc.close() and adding a regression test that detects close-time contention caused by eviction-driven shutdown under the XceiverClientManager cache monitor.
Changes:
- Switch
XceiverClientGrpc.close()from graceful shutdown + sleep polling toshutdownNow()+awaitTermination()to avoid the mandatory 100ms floor per close. - Add
TestXceiverClientManagerCloseContentionto assert eviction-driven closes do not serialize concurrent acquire/release activity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java | Reworks gRPC channel shutdown behavior in close() to eliminate the fixed sleep-based termination polling. |
| hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverClientManagerCloseContention.java | Adds a regression test that detects cache-eviction close contention and asserts acceptable per-close wall-clock cost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (ChannelInfo channelInfo : dnChannelInfoMap.values()) { | ||
| channelInfo.getChannel().shutdown(); | ||
| } | ||
|
|
||
| final long maxWaitNanos = TimeUnit.SECONDS.toNanos(SHUTDOWN_WAIT_MAX_SECONDS); | ||
| long deadline = System.nanoTime() + maxWaitNanos; | ||
| List<ManagedChannel> nonTerminatedChannels = dnChannelInfoMap.values() | ||
| .stream() | ||
| .map(ChannelInfo::getChannel) | ||
| .filter(Objects::nonNull) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| while (!nonTerminatedChannels.isEmpty() && System.nanoTime() < deadline) { | ||
| nonTerminatedChannels.removeIf(ManagedChannel::isTerminated); | ||
| ManagedChannel channel = channelInfo.getChannel(); | ||
| channel.shutdownNow(); | ||
| try { | ||
| Thread.sleep(SHUTDOWN_WAIT_INTERVAL_MILLIS); | ||
| if (!channel.awaitTermination(SHUTDOWN_WAIT_MAX_SECONDS, TimeUnit.SECONDS)) { | ||
| LOG.warn("Channel {} did not terminate within {}s.", channel, SHUTDOWN_WAIT_MAX_SECONDS); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| LOG.error("Interrupted while waiting for channels to terminate", e); | ||
| LOG.error("Interrupted while waiting for channel termination", e); | ||
| Thread.currentThread().interrupt(); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
its an old behavior that was supposed to be addressed by HDDS-14571, I'll file a new JIRA to get the better fix that does not introduce this regression.
|
Yeah this makes sense to me. |
|
Thanks @yandrey321 for the patch, @jojochuang for the review. |
What changes were proposed in this pull request?
XceiverClientGrpc.close() performs a blocking graceful channel shutdown, and because clients are torn down through XceiverClientManager's cache eviction, that blocking wait runs while the clientCache monitor is held — serializing every concurrent acquireClient() / releaseClient() call.
HDDS-14571 changed close() from a forced shutdownNow() (which cancels in-flight RPCs and terminates in ~1 ms) to a graceful shutdown() followed by a polling loop that always sleeps at least SHUTDOWN_WAIT_INTERVAL_MILLIS (100 ms) before it can observe termination:
Every close() now costs ≥100 ms instead of ~1 ms. The eviction path that invokes it holds the cache monitor throughout:
So concurrent readers that create/evict short-lived clients (e.g. EC read/checksum collection, which uses a distinct client per placement group) serialize behind a ≥100 ms lock hold per closed channel and degrade non-linearly with concurrency.
Fix: restore immediate termination in close() — use channel.shutdownNow() followed by channel.awaitTermination(SHUTDOWN_WAIT_MAX_SECONDS, ...), and drop the mandatory-sleep polling loop. This keeps HDDS-14571's goal (no synchronized methods on XceiverClientGrpc) while eliminating the blocking wait under the cache lock.
Co authored with Claude Code
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15849
How was this patch tested?
Added TestXceiverClientManagerCloseContention, a unit test that exercises the real XceiverClientManager and XceiverClientGrpc.close() (real lazily-connected plaintext gRPC channels to random local datanode addresses; no MiniOzoneCluster and no live datanode required, since close() only shuts channels down). maxCacheSize=1 plus a distinct pipeline per iteration forces an eviction-driven close on nearly every acquisition; 8 threads × 8 cycles produce 63 closes. It measures average wall-clock per close and asserts it stays well under the pre-fix ~100 ms floor.
The same test, unmodified, against the code before and after this change:
~34× faster per close